ESPHome trigger reconnect immediately when mDNS record received by OttoWinter · Pull Request #48129 · home-assistant/core

OttoWinter · 2021-03-19T21:25:26Z

Proposed change

Problem: ESPHome's native API has been difficult to use with the deep sleep feature, because home assistant only checks every 60s if the device is online again after sleep, so the device has to stay awake longer until HA connects and wasting power.

Idea: I had an idea yesterday: actually HA already knows when a device comes up again, because on startup the device broadcasts mDNS records on the network.

This PR: So this change implements an mDNS/zeroconf listener that triggers the reconnection logic immediately when such a DNS record is received.

In the process, I also refactored the logic into a new class ReconnectLogic because the code would have gotten messy otherwise. Note: not happy with the tries/connected/reconnect_event variables, but could not find more suitable synchronization primitives

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: none
This PR is related to issue: none
Link to documentation pull request: n/a

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver (I think?)
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

bdraco · 2021-03-20T00:40:30Z

Can we reload the config entry instead when it gets rediscovered via zeroconf?

#47683 now passes mdns updates and triggers zeroconf discovery

bdraco · 2021-03-20T00:42:54Z

Actually it looks like that will already happen now since #34753 updates the ip address and #47683 will allow the updates to come through.

OttoWinter · 2021-03-20T06:52:07Z

Hi,

Unfortunately, that does not work already and would not work.

The problem is that HA internally uses the zeroconf ServiceBrowser interface for catching these events. However, this interface only calls the handler when a new record is received and the matching old one is already expired (source), but the timescale we're working with here is often less than the TTL (with short deep sleep phases) but we would still like to receive the update. Hence also why I need to use RecordUpdateListener instead of ServiceBrowser, because I need direct access to the records coming in. I know it's not perfect, but I believe this is the best way for the logic to be informed of matching PTR records.

Also, it doesn't work already because the #34753 PR only updates the config entry data to set the host, but otherwise does not inform the reconnect logic.

If you're worried about resource usage with ZC, this is not an issue because the RecordUpdateListener interface really just forwards events from the Zeroconf object and does not start a new thread in constrast to ServiceBrowser.

bdraco · 2021-03-20T07:19:33Z

Hi,

Unfortunately, that does not work already and would not work.

The problem is that HA internally uses the zeroconf ServiceBrowser interface for catching these events. However, this interface only calls the handler when a new record is received and the matching old one is already expired (source), but the timescale we're working with here is often less than the TTL (with short deep sleep phases) but we would still like to receive the update. Hence also why I need to use RecordUpdateListener instead of ServiceBrowser, because I need direct access to the records coming in. I know it's not perfect, but I believe this is the best way for the logic to be informed of matching PTR records.

Also, it doesn't work already because the #34753 PR only updates the config entry data to set the host, but otherwise does not inform the reconnect logic.

If you're worried about resource usage with ZC, this is not an issue because the RecordUpdateListener interface really just forwards events from the Zeroconf object and does not start a new thread in constrast to ServiceBrowser.

Makes sense. Thanks for the through explanation.

bdraco

LGTM. Two small suggestions above.

OttoWinter · 2021-03-20T08:09:34Z

Ok, updated per suggestions. 👍
Also added a new wait_task variable to track the waiting task so that we don't delay HA shutdown.

bdraco · 2021-03-20T16:43:18Z

Looks good. Will do some manual testing later today

bdraco · 2021-03-21T00:48:31Z

2021-03-21 00:46:53 DEBUG (Thread-7) [homeassistant.components.dhcp] Processing updated address data for 192.168.106.243: mac=D8F15BD6030E hostname=maui_es015_tb_c

Lots of plugging and unplugging and it reconnects just about right after it gets an ip address 👍

This looks good

MartinHjelmare · 2021-03-21T02:31:50Z

+                if self._wait_task is not None:
+                    return
+
+                self._wait_task = self._hass.loop.create_task(


We can use asyncio.create_task instead.

MartinHjelmare · 2021-03-21T02:33:33Z

+        while True:
+            try:
+                await self._reconnect_once()
+            except asyncio.CancelledError:  # pylint: disable=try-except-raise


From Python 3.8 CancelledError inherits BaseException instead of Exception. So we don't need this case.

This is not to catch CancelledExceptions, but rather so that if the reconnect task is cancelled (meaning the reconnect loop should stop) we will actually cancel the loop, not just log the error and continue forever.

Yes, except Exception: won't catch asyncio.CancelledError.

Ah sorry, yes now I understand.

Thanks, didn't know that! Will fix this later.

MartinHjelmare · 2021-03-21T02:34:17Z

+        """Start the reconnecting logic background task."""
+        # Create reconnection loop outside of HA's tracked tasks in order
+        # not to delay startup.
+        self._loop_task = self._hass.loop.create_task(self._reconnect_loop())


Use asyncio.create_task.

OttoWinter added 2 commits March 19, 2021 21:59

ESPHome trigger reconnect immediately when mDNS record received

0295f64

Cleanup

5d79424

homeassistant added the cla-signed label Mar 19, 2021

probot-home-assistant Bot added integration: esphome new-feature by-code-owner labels Mar 19, 2021

bdraco reviewed Mar 20, 2021

View reviewed changes

Comment thread homeassistant/components/esphome/__init__.py Outdated

bdraco reviewed Mar 20, 2021

View reviewed changes

Comment thread homeassistant/components/esphome/__init__.py Outdated

bdraco approved these changes Mar 20, 2021

View reviewed changes

OttoWinter added 3 commits March 20, 2021 08:30

Small optimization

3ae32b7

Fix deprecated method call

49778c9

Also track wait task

919df67

Bre77 approved these changes Mar 20, 2021

View reviewed changes

Log errors from reconnect loop

e9d82df

bdraco reviewed Mar 20, 2021

View reviewed changes

Comment thread homeassistant/components/esphome/__init__.py Outdated

Apply suggestions

f949552

bdraco approved these changes Mar 21, 2021

View reviewed changes

bdraco merged commit 0193f16 into home-assistant:dev Mar 21, 2021

MartinHjelmare reviewed Mar 21, 2021

View reviewed changes

github-actions Bot locked and limited conversation to collaborators Mar 22, 2021

Uh oh!

Conversation

OttoWinter commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Additional information

Checklist

Uh oh!

bdraco commented Mar 20, 2021

Uh oh!

bdraco commented Mar 20, 2021

Uh oh!

OttoWinter commented Mar 20, 2021

Uh oh!

Uh oh!

Uh oh!

bdraco commented Mar 20, 2021

Uh oh!

bdraco left a comment

Choose a reason for hiding this comment

Uh oh!

OttoWinter commented Mar 20, 2021

Uh oh!

Uh oh!

bdraco commented Mar 20, 2021

Uh oh!

bdraco commented Mar 21, 2021

Uh oh!

MartinHjelmare Mar 21, 2021

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Mar 21, 2021

Choose a reason for hiding this comment

Uh oh!

OttoWinter Mar 21, 2021

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Mar 21, 2021

Choose a reason for hiding this comment

Uh oh!

OttoWinter Mar 21, 2021

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Mar 21, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

OttoWinter commented Mar 19, 2021 •

edited

Loading